Add new AvoidUsingNewObject rule#2186
Open
iRon7 wants to merge 5 commits into
Open
Conversation
…nd suggest type initializer as a fix.
iRon7
requested review from
a team,
bergmeister,
michaeltlombardi and
sdwheeler
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new PSScriptAnalyzer rule that warns on New-Object usage and provides auto-fix suggestions, along with documentation, localized strings, and Pester tests.
Changes:
- Added
AvoidUsingNewObjectrule implementation with suggested corrections/fixes. - Added rule documentation and registered it in the rules index.
- Added localization strings and comprehensive Pester tests for detection/suppression/fix behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/Rules/README.md | Registers the new rule in the documentation rules table. |
| docs/Rules/AvoidUsingNewObject.md | Adds rule reference documentation and examples. |
| Tests/Rules/AvoidUsingNewObject.tests.ps1 | Adds Pester coverage for rule behavior, suppression, and -fix. |
| Rules/Strings.resx | Adds localized strings for rule name/description/message/correction. |
| Rules/AvoidUsingNewObject.cs | Implements the rule logic and suggested corrections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <returns>A an enumerable type containing the violations</returns> | ||
| public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName) | ||
| { | ||
| if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); |
Comment on lines
+101
to
+112
| else if (boundResult.ConstantValue != null) | ||
| { | ||
| string valueText = boundResult.Value.Extent.Text; | ||
| if ( | ||
| boundResult.Value is StringConstantExpressionAst stringConstant && | ||
| stringConstant.StringConstantType == StringConstantType.BareWord | ||
| ) | ||
| { | ||
| valueText = '"' + valueText.Replace("\"", string.Empty) + '"'; // Test""123 --> "Test123" | ||
| } | ||
| correction = "[" + typeName + "]" + valueText; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
I think I got this right, if the -ArgumentList contains an constant value, I might use a shorter (preferred) syntax by simply casting the value expression.
For the given example: New-Object DateTime 0 vs [DateTime]0, both return the same results:
Monday, January 1, 0001 12:00:00 AM
Comment on lines
+113
to
+116
| else if ( | ||
| boundResult.Value is ParenExpressionAst parenExpressionAst && | ||
| !parenExpressionAst.Pipeline.Extent.Text.StartsWith(",") | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
The Pipeline.Extent.Text (the inner Ast of the ParenExpressionAst) is already trimmed.
Btw. I don't think there is an easier way to check for a wrapped object (unary comma operator).
iRon7
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2046
PR Summary
Avoid using the
New-Objectcmdlet to create objects as it might perform poorly.Instead, use type initializer to construct or cast the intended object.
Note
In most cases if there isn't an automatic correction isn't available,
the rule won't report any violation either.
This is because if there isn't an automatic correction available, it generally means
that there isn't a simple type-casting or type constructor that can be used that would
be used that would be more efficient or has a better syntax than using New-Object.
In other words, if the common
-Verboseparameter is used, or both the parameters-ArgumentListand-Propertyare used, there won't be a simple type initializeravailable and the rule won't report any violation for the
New-Objectcmdlet.Nevertheless, there are still some cases where the
New-Objectcmdlet might bereplaceable with a type initializer that would be more efficient or has a better syntax,
but an automatic correction can't be provided.
For example if the
-ArgumentListparameter is used with a variable,the rule will report a violation, but won't be able to provide a correction,
as it's not possible to determine from the AST alone whether the variable contains a
single value that can be used in a type initializer,
or if it contains multiple values that would require splatting.
This PR replaces PR #2109
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.